Skip to content

[NFC][SYCL][Graph] Add successors/predecessors views + cleanup #19332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 8, 2025

Conversation

aelovikov-intel
Copy link
Contributor

Part of refactoring to get rid of most (all?) std::weak_ptr<node_impl> and some of std::shared_ptr<node_impl> started in #19295. Use nodes_range from that PR to implement successors/predecessors views and update read-only accesses to the successors/predecessors to go through them.

I'm not changing the data members MSuccessors/MPredecessors yet because it would affect unittests. I'd prefer to refactor most of the code in future PRs before making that change and updating unittests in one go.

I'm updating some APIs to accept node_impl & instead of std::shared_ptr where the change is mostly localized to the callers iterating over successors/predecessors and doesn't spoil into other code too much. For those that weren't updated here we (temporarily) use shared_from_this() but I expect to eliminate those unnecessary copies when those interfaces will be updated in the subsequent PRs.

Part of refactoring to get rid of most (all?) `std::weak_ptr<node_impl>`
and some of `std::shared_ptr<node_impl>` started in
intel#19295. Use `nodes_range` from that PR
to implement `successors`/`predecessors` views and update read-only
accesses to the successors/predecessors to go through them.

I'm not changing the data members `MSuccessors`/`MPredecessors` yet
because it would affect unittests. I'd prefer to refactor most of the
code in future PRs before making that change and updating unittests in
one go.

I'm updating some APIs to accept `node_impl &` instead of
`std::shared_ptr` where the change is mostly localized to the callers
iterating over successors/predecessors and doesn't spoil into other code
too much. For those that weren't updated here we (temporarily) use
`shared_from_this()` but I expect to eliminate those unnecessary copies
when those interfaces will be updated in the subsequent PRs.
@aelovikov-intel aelovikov-intel requested a review from a team as a code owner July 7, 2025 20:13
@aelovikov-intel aelovikov-intel requested a review from Bensuo July 7, 2025 20:13
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 7, 2025
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 7, 2025
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 7, 2025
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025
... and update the code surrounding their uses in the same spirit.

Continuation of
intel#19295
intel#19332
intel#19334
@aelovikov-intel
Copy link
Contributor Author

@intel/sycl-graphs-reviewers , @Bensuo , would you be able to look at this before end of day your timezone? I have one uploaded PR and several I'm working on based on this one and I'd really like to get this merged soon.

@aelovikov-intel
Copy link
Contributor Author

Thanks!

@aelovikov-intel aelovikov-intel merged commit bb3498a into intel:sycl Jul 8, 2025
24 checks passed
@aelovikov-intel aelovikov-intel deleted the succ-pred-views branch July 8, 2025 15:41
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025
... and update the code surrounding their uses in the same spirit.

Continuation of
intel#19295
intel#19332
intel#19334
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025
... and update the code surrounding their uses in the same spirit.

Continuation of
intel#19295
intel#19332
intel#19334
aelovikov-intel added a commit that referenced this pull request Jul 8, 2025
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025
... and update the code surrounding their uses in the same spirit.

Continuation of
intel#19295
intel#19332
intel#19334
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025
... and update the code surrounding their uses in the same spirit.

Continuation of
intel#19295
intel#19332
intel#19334
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025
... and update the code surrounding their uses in the same spirit.

Continuation of
intel#19295
intel#19332
intel#19334
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025
Part of the refactoring to eliminate `std::weak_ptr<node_impl>` and
reduce usage of `std::shared_ptr<node_impl>` by preferring raw ptr/ref.
Previous PRs in the series:
intel#19295
intel#19332
intel#19334
intel#19350

* Accept `Deps` as `nodes_range` in `graph_impl::add`
* Return `node_impl &` from `graph_impl::add`
* Add `node` support in `nodes_range` and use that together with
  modified `graph_impl::add` when created new `node_impl`s based on
  `std::vector<node> Deps` to avoid creation of temporary `DepImpls`
  storage.
* Also updated `registerSuccessor/registerPredecessor` and
  `addEventForNode/addDepsToNode` to accept raw `node_impl &` as the
  changes above resulted in having raw reference at the call sites.
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025
Part of the refactoring to eliminate `std::weak_ptr<node_impl>` and
reduce usage of `std::shared_ptr<node_impl>` by preferring raw ptr/ref.
Previous PRs in the series:
intel#19295
intel#19332
intel#19334
intel#19350

* Accept `Deps` as `nodes_range` in `graph_impl::add`
* Return `node_impl &` from `graph_impl::add`
* Add `node` support in `nodes_range` and use that together with
  modified `graph_impl::add` when created new `node_impl`s based on
  `std::vector<node> Deps` to avoid creation of temporary `DepImpls`
  storage.
* Also updated `registerSuccessor/registerPredecessor` and
  `addEventForNode/addDepsToNode` to accept raw `node_impl &` as the
  changes above resulted in having raw reference at the call sites.
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025
Part of the refactoring to eliminate `std::weak_ptr<node_impl>` and
reduce usage of `std::shared_ptr<node_impl>` by preferring raw ptr/ref.
Previous PRs in the series:
intel#19295
intel#19332
intel#19334
intel#19350

* Accept `Deps` as `nodes_range` in `graph_impl::add`
* Return `node_impl &` from `graph_impl::add`
* Add `node` support in `nodes_range` and use that together with
  modified `graph_impl::add` when created new `node_impl`s based on
  `std::vector<node> Deps` to avoid creation of temporary `DepImpls`
  storage.
* Also updated `registerSuccessor/registerPredecessor` and
  `addEventForNode/addDepsToNode` to accept raw `node_impl &` as the
  changes above resulted in having raw reference at the call sites.
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025
Part of the refactoring to eliminate `std::weak_ptr<node_impl>` and
reduce usage of `std::shared_ptr<node_impl>` by preferring raw ptr/ref.
Previous PRs in the series:
intel#19295
intel#19332
intel#19334
intel#19350

* Accept `Deps` as `nodes_range` in `graph_impl::add`
* Return `node_impl &` from `graph_impl::add`
* Add `node` support in `nodes_range` and use that together with
  modified `graph_impl::add` when created new `node_impl`s based on
  `std::vector<node> Deps` to avoid creation of temporary `DepImpls`
  storage.
* Also updated `registerSuccessor/registerPredecessor` and
  `addEventForNode/addDepsToNode` to accept raw `node_impl &` as the
  changes above resulted in having raw reference at the call sites.
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants